packages/engine/internal/config/config_test.go (1)

313-333: Consider adding a test case for case normalization.

The test uses uppercase keys in the YAML (APP_NAME, REGION, DEBUG), which matches the expected output. However, since config.go normalizes keys to uppercase via strings.ToUpper, consider adding a test case with lowercase/mixed-case YAML keys to verify the normalization logic works correctly.

📋 Example additional test case

+func TestLoad_EnvMapCaseNormalization(t *testing.T) {
+	dir := t.TempDir()
+	configFile := filepath.Join(dir, "mantle.yaml")
+	err := os.WriteFile(configFile, []byte(`
+env:
+  app_name: "my-app"
+  Region: "us-east-1"
+`), 0644)
+	require.NoError(t, err)
+
+	cmd := newTestCommand()
+	_ = cmd.Flags().Set("config", configFile)
+
+	cfg, err := Load(cmd)
+	require.NoError(t, err)
+
+	// Keys should be normalized to uppercase
+	assert.Equal(t, "my-app", cfg.Env["APP_NAME"])
+	assert.Equal(t, "us-east-1", cfg.Env["REGION"])
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/config/config_test.go` around lines 313 - 333, The
test TestLoad_EnvMap currently only asserts behavior with already-uppercase YAML
keys; add an additional subcase that writes env keys in lowercase and mixed-case
(e.g., "app_name", "Region", "Debug") to the temporary mantle.yaml, call
Load(cmd) the same way, and assert that cfg.Env contains the normalized
uppercase keys ("APP_NAME", "REGION", "DEBUG") with the expected values to
verify the strings.ToUpper normalization in Load and the cfg.Env map behavior.

packages/engine/internal/cel/cel.go (1)

211-217: Document or synchronize envCache access to prevent future concurrency issues.

SetConfigEnv writes to e.envCache without synchronization while Eval reads from it. Current usage is safe—SetConfigEnv is called once during engine initialization (serve.go:52, run.go:67) before evaluations begin. However, to prevent data races if the API is used differently in the future, consider adding a sync.RWMutex or a comment documenting the initialization-only requirement.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/cel/cel.go` around lines 211 - 217, SetConfigEnv
writes to e.envCache without synchronization while Eval reads it, risking future
data races; add a sync.RWMutex field (e.g., envMu) to the Evaluator struct and
use envMu.Lock()/Unlock() in SetConfigEnv and envMu.RLock()/RUnlock() in Eval
(and any other readers) when accessing e.envCache, or if you prefer the
lightweight option, add a clear comment on the Evaluator type stating envCache
is initialization-only and must not be mutated after startup; reference
SetConfigEnv, Eval, Evaluator, envCache, and mergeEnvVars when making the
change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/engine/internal/cli/run.go (1)

67-68: Minor ordering inconsistency with serve.go.

In serve.go (lines 51-54), SetConfigEnv is called immediately after engine.New, before setting MaxConcurrentExecutionsPerTeam. Here the order is reversed. While functionally equivalent, aligning the order improves consistency across CLI commands.

🔧 Suggested reordering for consistency

 		eng, err := engine.New(database)
 		if err != nil {
 			return fmt.Errorf("creating engine: %w", err)
 		}
+		eng.CEL.SetConfigEnv(cfg.Env)
 		eng.MaxConcurrentExecutionsPerTeam = cfg.Engine.MaxConcurrentExecutionsPerTeam
-		eng.CEL.SetConfigEnv(cfg.Env)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/cli/run.go` around lines 67 - 68, Reorder the two
assignments so they match serve.go: after creating the engine via engine.New,
call eng.CEL.SetConfigEnv(cfg.Env) immediately, then set
eng.MaxConcurrentExecutionsPerTeam = cfg.Engine.MaxConcurrentExecutionsPerTeam;
adjust the sequence around the eng variable (engine.New, eng.CEL.SetConfigEnv,
then setting MaxConcurrentExecutionsPerTeam) to keep CLI command initialization
consistent.

packages/engine/internal/cel/cel.go (1)

227-233: Consider thread-safety for SetConfigEnv if called concurrently.

envCache is read in Eval (line 88) without synchronization while SetConfigEnv writes to it. Current usage (init-time only in CLI commands) is safe, but if SetConfigEnv is ever called during concurrent execution, this becomes a data race.

If dynamic reconfiguration is planned, consider adding a sync.RWMutex to protect envCache. Otherwise, documenting the "call only during initialization" constraint would be sufficient.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/cel/cel.go` around lines 227 - 233, The SetConfigEnv
method writes to Evaluator.envCache while Eval reads it without synchronization,
creating a potential data race; add a sync.RWMutex field (e.g., envMu) to the
Evaluator struct and use envMu.Lock()/Unlock() in SetConfigEnv when assigning
e.configEnv and e.envCache (and when calling mergeEnvVars), and use
envMu.RLock()/RUnlock() in Eval where it reads e.envCache to protect concurrent
access; if you prefer not to change runtime behavior, instead add a clear
comment on Evaluator.SetConfigEnv and Eval stating that SetConfigEnv must only
be called during initialization.

packages/engine/internal/cel/cel_test.go (1)

300-304: Prefer t.Setenv over manual os.Setenv/defer os.Unsetenv.

Using t.Setenv provides automatic cleanup and is parallel-safe. The manual approach works but is more error-prone.

♻️ Suggested refactor

 func TestEnvVars_PrefixStripping(t *testing.T) {
 	// Directly test the mergeEnvVars function.
-	os.Setenv("MANTLE_ENV_TEST_KEY", "test-value")
-	defer os.Unsetenv("MANTLE_ENV_TEST_KEY")
-
-	os.Setenv("MANTLE_DATABASE_URL", "should-not-appear")
-	defer os.Unsetenv("MANTLE_DATABASE_URL")
+	t.Setenv("MANTLE_ENV_TEST_KEY", "test-value")
+	t.Setenv("MANTLE_DATABASE_URL", "should-not-appear")

 	result := mergeEnvVars(nil)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/engine/internal/cel/cel_test.go` around lines 300 - 304, In the test
in cel_test.go that currently calls os.Setenv("MANTLE_ENV_TEST_KEY", ...) and
os.Setenv("MANTLE_DATABASE_URL", ...) with deferred os.Unsetenv calls, replace
those manual environment manipulations with t.Setenv("MANTLE_ENV_TEST_KEY",
"test-value") and t.Setenv("MANTLE_DATABASE_URL", "should-not-appear")
respectively and remove the corresponding defer os.Unsetenv lines so the
testing.T-managed cleanup (and parallel-safety) is used instead.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes